fix: route converter-atomic JSON-object nodes through TryModify in TryPatch#85
Merged
Conversation
…yPatch
Reflector.TryPatch structurally descended into every JSON-object patch node,
enumerating its keys as members to navigate. A value whose JSON encoding is an
object but which a registered converter resolves indivisibly (e.g. a Unity
object reference {"instanceID":"…"}) was therefore navigated key-by-key and
failed with "Segment 'instanceID' not found on type '<T>'" instead of being
delegated to the converter that knows how to resolve it.
Add a default-false capability flag bool TreatJsonObjectAsAtomicValue(Type) to
IReflectionConverter (virtual false in BaseReflectionConverter). In
TryPatchInternal, after the leaf checks and before the $type / key-enumeration
block, a JSON-object node whose target type has a converter reporting the flag
true and that carries no "$type" key is routed through TryModify — the same
converter path componentDiff and pathPatches use. Plain POCO sub-objects (flag
false) still descend structurally; $type polymorphic replacement still flows
through the existing structural path; non-object leaves are unchanged.
The Unity-side one-line override of the flag is tracked separately (downstream
task) and is not part of this change.
Closes #84
verified-downstream: McpPlugin.Tests, McpPlugin.Server.Tests, Unity-EditMode
Contributor
…olution
Add a negative test: when a converter-atomic JSON-object node is reached but
its SetValue resolution fails (unresolvable {"ref":"missing-id"}), TryPatch
must return false and surface the converter's own error — it must NOT fall
through to structural key descent, which would re-produce the original #84
failure ("Segment 'ref' not found on type 'AssetRef'"). Asserts the failure
return, the converter error in logs, and the absence of a "not found"
structural-descent message.
verified-downstream: none (test-only; library DLL unchanged)
…s (positional)
Make the converter-atomic JSON-object delegation positional: it fires ONLY for
non-root nodes (depth > 0), i.e. a value being assigned into a member/element
slot. The ROOT node (depth 0) is always patched STRUCTURALLY — the target
object is modified in place by descending into its members.
This removes the need for any type-based scoping in the delegation: a multi-field
patch of an object (including a Unity component) at the root still applies
field-by-field, while a reference encoded as {"instanceID":…} assigned INTO a
member slot is resolved atomically by its converter. Downstream (Unity) the
capability override can therefore be a clean blanket true with no Component /
GameObject exclusion, and a component-reference field by {instanceID} now
resolves as well.
Tests: root-is-structural contract (a'), positional crux proving the same
atomic type is structural at root and atomic as a member (a''), plus the
existing member-depth atomic + failed-resolution cases. All TryPatch_* /
AtomicModifyTests remain green on net8.0 + net9.0.
verified-downstream: pending Unity verification (separate verifier pass)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reflector.TryPatchdescended structurally into every JSON-object patch node, so a value whose JSON encoding is an object but which a registered converter resolves indivisibly (e.g. a Unity object reference{"instanceID":"…"}) failed withSegment 'instanceID' not found on type '<T>'instead of being delegated to its converter.bool TreatJsonObjectAsAtomicValue(Type)toIReflectionConverter(virtualfalseinBaseReflectionConverter).TryPatchInternal, after the leaf checks and before the$type/ key-enumeration block, a JSON-object node whose target type has a converter reporting the flagtrueand that carries no$typekey is routed throughTryModify— the same converter path componentDiff/pathPatches use. Plain POCO descent and$typepolymorphic replacement are unchanged.The Unity-side one-line override of the flag is a separate downstream task and is not part of this PR.
Test plan
TryPatchAtomicConverterTests, Unity-agnostic stub converter) cover: converter-atomic object-node routing (member + root), unchanged POCO structural descent,$typereplacement, non-object leaf — 5/5 green on net8.0 + net9.0.Closes #84